Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded readiness guards and connection-loss handling to the ZooKeeper provider; computed and tracked actual subscription node paths for global/tenant/personal targets. Tightened library service readiness checks and file-read error handling; ensured explicit file-handle cleanup when loading favorites and disabled lists; updated docs describing readiness and subscribe contract. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
asab/library/service.py (1)
538-574: Consider logging the suppressed close() exception.The control flow correctly separates read vs parse failures: read failures preserve existing state (appropriate for transient errors), while parse failures reset to empty (appropriate for malformed config).
However, the
try/except/passat lines 570-573 silently swallows any close exception. While this is defensive cleanup code, logging at DEBUG level would aid troubleshooting.♻️ Optional: Log suppressed close exception
finally: if hasattr(disabled_file, "close"): try: disabled_file.close() except Exception: - pass + L.debug("Failed to close disabled_file handle")The same pattern applies to
_read_favoritesat lines 471-474.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/service.py` around lines 538 - 574, The finally block that closes disabled_file currently swallows exceptions silently; update it to log any exception at DEBUG (or similar) so suppressed close errors are visible during troubleshooting—capture the exception when calling disabled_file.close() and call the module logger (e.g. L.debug or L.exception with a brief message) instead of a bare pass; apply the same change to the analogous cleanup in _read_favorites so both close failures are logged while preserving existing control flow and behavior for Disabled and DisabledPaths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@asab/library/service.py`:
- Around line 538-574: The finally block that closes disabled_file currently
swallows exceptions silently; update it to log any exception at DEBUG (or
similar) so suppressed close errors are visible during troubleshooting—capture
the exception when calling disabled_file.close() and call the module logger
(e.g. L.debug or L.exception with a brief message) instead of a bare pass; apply
the same change to the analogous cleanup in _read_favorites so both close
failures are logged while preserving existing control flow and behavior for
Disabled and DisabledPaths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b818412-8126-43fd-8bfb-04f47a1823e3
📒 Files selected for processing (2)
asab/library/providers/zookeeper.pyasab/library/service.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/providers/zookeeper.py`:
- Around line 498-505: The current subscribe() branch for personal targets
caches an empty list when self._subscription_personal_path(path, target) returns
None, causing _on_library_changed() to treat it as resolved and never retry;
instead, change the logic in the subscribe path (the branch that calls
self._subscription_personal_path and currently assigns [] to
self.SubscriptionActualPaths[(target, path)]) to NOT cache an empty list —
either delete the key or store a sentinel (e.g., None or a dedicated unresolved
marker) when actual_path is None, and update _on_library_changed() to treat that
sentinel/absent key as unresolved (triggering recomputation of
self._subscription_personal_path and re-evaluation of NodeDigests) so personal
subscriptions are retried when context becomes available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c824d87-f9cd-40d8-a237-f9541cf2e824
📒 Files selected for processing (1)
asab/library/providers/zookeeper.py
| elif target == "personal" or (isinstance(target, tuple) and len(target) == 2 and target[0] == "personal"): | ||
| actual_path = self._subscription_personal_path(path, target) | ||
| if actual_path is not None: | ||
| self.SubscriptionActualPaths[(target, path)] = [actual_path] | ||
| self.NodeDigests[actual_path] = await self._get_directory_hash(actual_path) | ||
| else: | ||
| self.SubscriptionActualPaths[(target, path)] = [] | ||
| else: |
There was a problem hiding this comment.
Personal subscriptions can get stuck permanently when context is missing at subscribe time.
If tenant/credentials are unavailable, subscribe() stores [] for (target, path). Later, _on_library_changed() treats that as a valid cached value and never recomputes, so updates may never be observed.
💡 Proposed fix
@@
elif target == "personal" or (isinstance(target, tuple) and len(target) == 2 and target[0] == "personal"):
actual_path = self._subscription_personal_path(path, target)
if actual_path is not None:
self.SubscriptionActualPaths[(target, path)] = [actual_path]
self.NodeDigests[actual_path] = await self._get_directory_hash(actual_path)
else:
- self.SubscriptionActualPaths[(target, path)] = []
+ # Keep unresolved; allow late resolution during change checks.
+ self.SubscriptionActualPaths.pop((target, path), None)
@@
elif target == "personal" or (isinstance(target, tuple) and len(target) == 2 and target[0] == "personal"):
actual_paths = subscription_actual_paths.get((target, path))
- if actual_paths is None:
+ if not actual_paths:
actual_path = self._subscription_personal_path(path, target)
actual_paths = [actual_path] if actual_path is not None else []
+ if actual_paths:
+ self.SubscriptionActualPaths[(target, path)] = actual_paths
for actual_path in actual_paths:Also applies to: 575-580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/providers/zookeeper.py` around lines 498 - 505, The current
subscribe() branch for personal targets caches an empty list when
self._subscription_personal_path(path, target) returns None, causing
_on_library_changed() to treat it as resolved and never retry; instead, change
the logic in the subscribe path (the branch that calls
self._subscription_personal_path and currently assigns [] to
self.SubscriptionActualPaths[(target, path)]) to NOT cache an empty list —
either delete the key or store a sentinel (e.g., None or a dedicated unresolved
marker) when actual_path is None, and update _on_library_changed() to treat that
sentinel/absent key as unresolved (triggering recomputation of
self._subscription_personal_path and re-evaluation of NodeDigests) so personal
subscriptions are retried when context becomes available.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asab/library/service.py`:
- Around line 545-569: The code currently writes directly to self.Disabled and
self.DisabledPaths while parsing /.disabled.yaml, which can clear the
last-known-good state on parse errors and leave scalar tenant entries as raw
strings; instead parse and normalize into temporary locals (e.g., tmp_disabled =
{} and tmp_disabled_paths = []), validate the payload (handle None, handle set
by converting each member to tmp_disabled[member] = '*', ensure mapping values
are appropriate), normalize scalar/mapping entries so tenants are exact keys
(not substring matches) and route keys ending with '/' into tmp_disabled_paths
as (key, value), and only after successful validation assign self.Disabled =
tmp_disabled and self.DisabledPaths = tmp_disabled_paths; reference
check_disabled and _is_disabled_diff_affecting_path to ensure exact-tenant
semantics are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 267a0454-2fe2-4aee-a541-d11c0dcd04fb
📒 Files selected for processing (2)
asab/library/service.pydocs/reference/services/library.md
| try: | ||
| disabled_data = yaml.load(disabled_file, Loader=yaml.CSafeLoader) | ||
| except Exception: | ||
| L.exception("Failed to parse '/.disabled.yaml'") | ||
| self.Disabled = {} | ||
| self.DisabledPaths = [] | ||
| return | ||
|
|
||
| if disabled_data is None: | ||
| self.Disabled = {} | ||
| self.DisabledPaths = [] | ||
| return | ||
|
|
||
| if isinstance(disabled_data, set): | ||
| # Backward compatibility (August 2023) | ||
| self.Disabled = {key: '*' for key in disabled_data} | ||
| self.DisabledPaths = [] | ||
| else: | ||
| self.Disabled = {} | ||
| self.DisabledPaths = [] | ||
| for k, v in disabled_data.items(): | ||
| if k.endswith('/'): | ||
| self.DisabledPaths.append((k, v)) | ||
| else: | ||
| self.Disabled[k] = v |
There was a problem hiding this comment.
Validate and normalize /.disabled.yaml before swapping shared state.
This branch replaces self.Disabled / self.DisabledPaths before the payload is fully validated. That creates two correctness problems: malformed YAML clears the last known-good disable map, and scalar entries like tenant-1 stay as raw strings, so check_disabled() / _is_disabled_diff_affecting_path() fall back to substring and character-based membership instead of exact tenant matching. Build a temporary normalized map first, then assign it only after validation succeeds.
Proposed fix
- try:
- disabled_data = yaml.load(disabled_file, Loader=yaml.CSafeLoader)
- except Exception:
- L.exception("Failed to parse '/.disabled.yaml'")
- self.Disabled = {}
- self.DisabledPaths = []
- return
-
- if disabled_data is None:
- self.Disabled = {}
- self.DisabledPaths = []
- return
-
- if isinstance(disabled_data, set):
- # Backward compatibility (August 2023)
- self.Disabled = {key: '*' for key in disabled_data}
- self.DisabledPaths = []
- else:
- self.Disabled = {}
- self.DisabledPaths = []
- for k, v in disabled_data.items():
- if k.endswith('/'):
- self.DisabledPaths.append((k, v))
- else:
- self.Disabled[k] = v
+ try:
+ disabled_data = yaml.load(disabled_file, Loader=yaml.CSafeLoader)
+ except Exception:
+ L.exception("Failed to parse '/.disabled.yaml'")
+ return
+
+ if disabled_data is None:
+ new_disabled = {}
+ new_disabled_paths = []
+ elif isinstance(disabled_data, set):
+ # Backward compatibility (August 2023)
+ new_disabled = {key: ["*"] for key in disabled_data}
+ new_disabled_paths = []
+ elif isinstance(disabled_data, dict):
+ new_disabled = {}
+ new_disabled_paths = []
+ for k, v in disabled_data.items():
+ if not isinstance(k, str):
+ L.warning("Ignoring non-string disabled key: {}".format(k))
+ continue
+
+ if isinstance(v, (list, tuple, set)):
+ targets = [str(t) for t in v]
+ elif v is None:
+ targets = []
+ else:
+ targets = [str(v)]
+
+ if k.endswith('/'):
+ new_disabled_paths.append((k, targets))
+ else:
+ new_disabled[k] = targets
+ else:
+ L.warning(
+ "Unexpected disabled format ({}). Keeping previous state.".format(
+ type(disabled_data).__name__
+ )
+ )
+ return
+
+ self.Disabled = new_disabled
+ self.DisabledPaths = new_disabled_paths📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| disabled_data = yaml.load(disabled_file, Loader=yaml.CSafeLoader) | |
| except Exception: | |
| L.exception("Failed to parse '/.disabled.yaml'") | |
| self.Disabled = {} | |
| self.DisabledPaths = [] | |
| return | |
| if disabled_data is None: | |
| self.Disabled = {} | |
| self.DisabledPaths = [] | |
| return | |
| if isinstance(disabled_data, set): | |
| # Backward compatibility (August 2023) | |
| self.Disabled = {key: '*' for key in disabled_data} | |
| self.DisabledPaths = [] | |
| else: | |
| self.Disabled = {} | |
| self.DisabledPaths = [] | |
| for k, v in disabled_data.items(): | |
| if k.endswith('/'): | |
| self.DisabledPaths.append((k, v)) | |
| else: | |
| self.Disabled[k] = v | |
| try: | |
| disabled_data = yaml.load(disabled_file, Loader=yaml.CSafeLoader) | |
| except Exception: | |
| L.exception("Failed to parse '/.disabled.yaml'") | |
| return | |
| if disabled_data is None: | |
| new_disabled = {} | |
| new_disabled_paths = [] | |
| elif isinstance(disabled_data, set): | |
| # Backward compatibility (August 2023) | |
| new_disabled = {key: ["*"] for key in disabled_data} | |
| new_disabled_paths = [] | |
| elif isinstance(disabled_data, dict): | |
| new_disabled = {} | |
| new_disabled_paths = [] | |
| for k, v in disabled_data.items(): | |
| if not isinstance(k, str): | |
| L.warning("Ignoring non-string disabled key: {}".format(k)) | |
| continue | |
| if isinstance(v, (list, tuple, set)): | |
| targets = [str(t) for t in v] | |
| elif v is None: | |
| targets = [] | |
| else: | |
| targets = [str(v)] | |
| if k.endswith('/'): | |
| new_disabled_paths.append((k, targets)) | |
| else: | |
| new_disabled[k] = targets | |
| else: | |
| L.warning( | |
| "Unexpected disabled format ({}). Keeping previous state.".format( | |
| type(disabled_data).__name__ | |
| ) | |
| ) | |
| return | |
| self.Disabled = new_disabled | |
| self.DisabledPaths = new_disabled_paths |
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 547-547: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asab/library/service.py` around lines 545 - 569, The code currently writes
directly to self.Disabled and self.DisabledPaths while parsing /.disabled.yaml,
which can clear the last-known-good state on parse errors and leave scalar
tenant entries as raw strings; instead parse and normalize into temporary locals
(e.g., tmp_disabled = {} and tmp_disabled_paths = []), validate the payload
(handle None, handle set by converting each member to tmp_disabled[member] =
'*', ensure mapping values are appropriate), normalize scalar/mapping entries so
tenants are exact keys (not substring matches) and route keys ending with '/'
into tmp_disabled_paths as (key, value), and only after successful validation
assign self.Disabled = tmp_disabled and self.DisabledPaths = tmp_disabled_paths;
reference check_disabled and _is_disabled_diff_affecting_path to ensure
exact-tenant semantics are preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/services/library.md`:
- Around line 155-161: Replace the awkward heading "current merged `subscribe()`
target contract" with a concise "subscribe() target contract" and tighten the
four bullets to avoid repeating "watches" by using short, parallel phrases;
e.g., keep the symbol `subscribe()` and reword the bullets so they read like:
`target=None` or `target="global"` — Global path, `target="tenant"` —
Tenant-scoped paths, `target=("tenant", "<tenant-id>")` — Specific tenant,
`target="personal"` — Current personal scope (tenant + credentials),
`target=("personal", "<credentials-id>")` — Specific personal scope under
current tenant. Ensure `subscribe()` and each target tuple string are preserved
exactly as shown so links/search still find them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a5856e2-189d-46e5-8c8c-8597cf04f722
📒 Files selected for processing (1)
docs/reference/services/library.md
| The current merged `subscribe()` target contract is: | ||
|
|
||
| - `target=None` or `target="global"` watches the global path. | ||
| - `target="tenant"` watches tenant-scoped paths. | ||
| - `target=("tenant", "<tenant-id>")` watches one explicit tenant scope. | ||
| - `target="personal"` watches the current `(tenant, credentials)` context. | ||
| - `target=("personal", "<credentials-id>")` watches one explicit personal scope under the current tenant. |
There was a problem hiding this comment.
Tighten wording in target bullets for readability.
This section is accurate, but phrasing is a bit repetitive (“watches …” on every line) and “current merged subscribe() target contract” reads awkwardly. Consider shortening and varying sentence starts for easier scanning.
✍️ Suggested doc polish
-The current merged `subscribe()` target contract is:
+The `subscribe()` target contract is:
- `target=None` or `target="global"` watches the global path.
-- `target="tenant"` watches tenant-scoped paths.
-- `target=("tenant", "<tenant-id>")` watches one explicit tenant scope.
-- `target="personal"` watches the current `(tenant, credentials)` context.
-- `target=("personal", "<credentials-id>")` watches one explicit personal scope under the current tenant.
+- `target="tenant"` covers tenant-scoped paths.
+- `target=("tenant", "<tenant-id>")` limits watching to one tenant scope.
+- `target="personal"` uses the current `(tenant, credentials)` context.
+- `target=("personal", "<credentials-id>")` watches one personal scope under the current tenant.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The current merged `subscribe()` target contract is: | |
| - `target=None` or `target="global"` watches the global path. | |
| - `target="tenant"` watches tenant-scoped paths. | |
| - `target=("tenant", "<tenant-id>")` watches one explicit tenant scope. | |
| - `target="personal"` watches the current `(tenant, credentials)` context. | |
| - `target=("personal", "<credentials-id>")` watches one explicit personal scope under the current tenant. | |
| The `subscribe()` target contract is: | |
| - `target=None` or `target="global"` watches the global path. | |
| - `target="tenant"` covers tenant-scoped paths. | |
| - `target=("tenant", "<tenant-id>")` limits watching to one tenant scope. | |
| - `target="personal"` uses the current `(tenant, credentials)` context. | |
| - `target=("personal", "<credentials-id>")` watches one personal scope under the current tenant. |
🧰 Tools
🪛 LanguageTool
[style] ~160-~160: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cit tenant scope. - target="personal" watches the current (tenant, credentials) con...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~161-~161: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...arget=("personal", "")` watches one explicit personal scope under the c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/services/library.md` around lines 155 - 161, Replace the
awkward heading "current merged `subscribe()` target contract" with a concise
"subscribe() target contract" and tighten the four bullets to avoid repeating
"watches" by using short, parallel phrases; e.g., keep the symbol `subscribe()`
and reword the bullets so they read like: `target=None` or `target="global"` —
Global path, `target="tenant"` — Tenant-scoped paths, `target=("tenant",
"<tenant-id>")` — Specific tenant, `target="personal"` — Current personal scope
(tenant + credentials), `target=("personal", "<credentials-id>")` — Specific
personal scope under current tenant. Ensure `subscribe()` and each target tuple
string are preserved exactly as shown so links/search still find them.
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation